[Console] Improve UX around handling multiple requests#132494
[Console] Improve UX around handling multiple requests#132494mibragimov merged 25 commits intoelastic:mainfrom
Conversation
…-ref HEAD~1..HEAD --fix'
mibragimov
added
release_note:enhancement
Feature:Console
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
Verified functional tests through flaky-test-suite-runner https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/670 |
|
Pinging @elastic/platform-deployment-management (Team:Deployment Management) |
cjcenizal
left a comment
cjcenizal
left a comment
There was a problem hiding this comment.
@elastic/kibana-design Would you or some folks from EUI please review the styles here? I want to make sure they're correct, especially for dark mode.
@mibragimov People are going to love this enhancement! Thank you so much for making this change. I had one suggestion to add some more tests, other than that this code LGTM.
Did a quick test locally and things look good there too:
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
expected head sha didn’t match current head ref. |
ping @elastic/kibana-design |
MichaelMarcialis
left a comment
MichaelMarcialis
left a comment
There was a problem hiding this comment.
I've left a few style comments below for your review. Thanks!
| background-color: $euiColorSuccess; | ||
| color: chooseLightOrDarkText($euiColorSuccess); |
There was a problem hiding this comment.
Suggesting change to the correct badge background colors here.
| background-color: $euiColorSuccess; | |
| color: chooseLightOrDarkText($euiColorSuccess); | |
| background-color: $euiColorVis0_behindText; | |
| color: chooseLightOrDarkText($euiColorVis0_behindText); |
There was a problem hiding this comment.
@MichaelMarcialis For our own edification, can you teach us about the role of $euiColorVis0_behindText and similar colors, and what makes it better than $euiColorSuccess here?
There was a problem hiding this comment.
Good question, @cjcenizal! For the purposes of this PR, my reason for the suggested color changes were purely to make these ace badges consistent with the colors that our EuiBadge components use.
That said, I imagine the reason the EUI team ultimately chose to use the behind-text visualization colors for the EuiBadge component has to do with accessibility and the contrast ratio, as this component houses fairly small sized text. The contrast with the behind-text visualization colors appears to be higher and receives a better WCAG rating. Unlike EuiBadge, I assume that components with larger text sizes (such as EuiButton) can afford be a bit less strict with the color contrast, which is why they can use colors like $euiColorSuccess.
CCing @cchaos here to keep me honest and double-check my assumptions.
There was a problem hiding this comment.
For full context on why we used visualization colors for badges, you can read through this PR: elastic/eui#2455
There was a problem hiding this comment.
Thank you both! This helps me out a ton.
|
@elasticmachine merge upstream |
|
@MichaelMarcialis thanks for the review! I've addressed your feedback with ad57319, do you mind having another look? |
There was a problem hiding this comment.
Thanks for making those changes @mibragimov. I only have one additional thought. Would it be worth changing the font family of .ace_badge to $euiFontFamily in this case? Currently, the .ace_badges are inheriting the editor font family, which differentiates itself from the standard EUI badges. Assuming these badges only ever appear at the end of editor lines, I think this might look nicer/more consistent.
Otherwise, this looks good from my perspective. Approving now so that I don't hold you up further. Thanks.
|
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @mibragimov |
8 participants
Fixes #130982
This PR adds support for highlighting the results for multiple requests in the editor output